-
Notifications
You must be signed in to change notification settings - Fork 5
feat: add marketplace button to eID Wallet page #790
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughReplaces an eVault IdentityCard with a W3DS marketplace discovery Button.Nav, swaps an icon import to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@infrastructure/eid-wallet/src/routes/`(app)/main/+page.svelte:
- Line 75: Remove the leftover debug console.log by deleting the statement that
logs "status current" and profileCreationStatus in the +page.svelte component
(the console.log("status current", profileCreationStatus) line); ensure no other
debugging console statements remain in the component and, if needed for
production, replace with a structured log via the app's logger or remove
entirely to avoid leaking internal state.
- Around line 176-188: The hover style on the Button.Nav component causes white
text to become unreadable because hover:bg-gray-200 lightens the background;
update the Button.Nav class list (the component instance using Button.Nav and
HugeiconsIcon) to use a darker hover background such as hover:bg-black-600 or
hover:bg-gray-800 (or alternatively add a contrasting hover text class like
hover:text-black) so the text remains legible on hover.
- Around line 62-70: The async IIFE inside onMount that awaits
globalState.userController.user, globalState.userController.isFake, and
globalState.vaultController.vault can reject and currently has no error
handling; wrap the async function body in a try/catch (or append a .catch()) to
handle rejections, set/clear userData and ename appropriately on error, and log
or surface the error (e.g., via console.error or an existing logger) so failures
from userController.user, userController.isFake or vaultController.vault are not
unhandled.
- Around line 176-178: The external link using the Button.Nav component should
open in a new tab and be secured; add target="_blank" and rel="noopener
noreferrer" to the Button.Nav invocation that links to
"https://marketplace.w3ds.metastate.foundation/". Ensure the Button.Nav
component (the anchor wrapper) forwards arbitrary attributes to the underlying
<a> (e.g., via $$restProps or equivalent) so target and rel are rendered; if it
doesn't, update Button.Nav to accept and pass through these attributes before
adding them to the instance in +page.svelte.
🧹 Nitpick comments (3)
infrastructure/eid-wallet/src/routes/(app)/main/+page.svelte (3)
13-14: Nit: consolidate imports from the same module.
onDestroycan be merged into the import on line 13.- import { type Snippet, getContext, onMount } from "svelte"; - import { onDestroy } from "svelte"; + import { type Snippet, getContext, onDestroy, onMount } from "svelte";
25-26:statusIntervaldoesn't need$state— it's never used in the template.A plain
letis sufficient for the interval handle since it's only read/written imperatively inonMount/onDestroy.- let statusInterval: ReturnType<typeof setInterval> | undefined = - $state(undefined); + let statusInterval: ReturnType<typeof setInterval> | undefined = undefined;
77-84: Consider makingprofileCreationStatusreactive instead of polling.Polling every 1 second is a pragmatic workaround, but if
vaultControllercan expose a reactive store or signal, that would be cleaner and more efficient. If that's not feasible right now, this works fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@infrastructure/eid-wallet/src/routes/`(app)/main/+page.svelte:
- Around line 180-184: Replace the generic alt text on the img element used in
the Discover header by updating the alt attribute on the <img class="w-12"
src="/images/W3DSLogoWhite.svg" ...> element from "logo" to a more descriptive
label like "W3DS Logo" (or "W3DS") so screen readers convey meaningful
information about the image.
Description of change
Added a button link to marketplace on the main eid wallet page
Issue Number
Type of change
How the change has been tested
Change checklist
Summary by CodeRabbit
New Features
Style